Migrate Post Template Delete button from `confirm()` to `ConfirmDialog` #37535

chad1008 · 2021-12-20T20:08:06Z

Related: #34153

Description

This PR aims to migrate the FSE post template editor's Delete template menu item away from the current confirm() implementation and instead use the new experimental ConfirmDialog component.

How has this been tested?

Before testing, cherrypick #37959 on top of this PR. That will ensure the ConfirmDialog has the proper z-index to render on top of its parent component.

Running WordPress 5.8.2 via wp-env:

With a block-based theme active, create a new post with some basic content
In the editor sidebar, locate the Post Template section, and use the provided link to create a new template
Make any changes you'd like to the template and save it
Click the provided button to edit your new template
At the top of the editor open the dropdown that currently shows the name of your template
Click the Delete template button. Confirm no unexpected console errors appear.
Click the Cancel button. Confirm that the template remains intact.
Click Delete template again. This time, click OK and confirm that the template is deleted by trying to re-apply it to the post.

I've tested in the latest Chrome, Firefox, and Safari.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

chad1008 · 2022-01-07T21:36:50Z

Updated this PR to simplify the implementation of ConfirmDialog (cc @ciampo)

ciampo

Hey @chad1008 , thank you for working on this!

I noticed that the dropdown with the "Delete template" button sits on top of the confirm dialog. This happens at all screen sizes, but it can be more easily spotted on smaller viewports:

Otherwise, my feedback is basically the same as the one I left in PR 37491, including the consideration about unit/e2e tests.

packages/edit-post/src/components/header/template-title/delete-template.js

chad1008 · 2022-01-13T22:01:03Z

I noticed that the dropdown with the "Delete template" button sits on top of the confirm dialog. This happens at all screen sizes, but it can be more easily spotted on smaller viewports

@ciampo Well spotted. It turned out this was due to the current z-index settings for Popover and Modal components (ConfirmDialog is rendered by a Popover, and in turn renders a Modal). Currently, with limited exceptions, Popovers render on top of pretty much everything else.. I'm not familiar with the specifics behind that decision's history, but because ConfirmDialog is getting rendered by Popovers, we need it boost its z-index: #37959

Until that PR is merged, and this one is rebased, we should cherrypick that one into any further ConfirmDialog testing.

ciampo · 2022-01-21T09:28:43Z

Update:

I marked all of the memoization-related conversations as resolved for now, as discussed in this comment.
there's still an open comment that needs addressing
not sure if you managed to look into fixing any broken e2e/unit tests (if any) and potentially adding any new ones

It turned out this was due to the current z-index settings for Popover and Modal components [....] we need it boost its z-index: #37959

Thank you for looking into the problem and opening #37959. Unfortunately I'm afraid that the fix may need to be more complicated than bumping the z-index (as discussed in that PR).

We'll likely have to wait until this issue is fixed before being able to merge this PR

ciampo · 2022-02-04T15:14:52Z

With #37959 merged, we can consider this PR unblocked.

@chad1008 , feel free to continue working on this PR (I believe we're only missing any potential e2e test work) and ping me when it's ready for the next round of review!

chad1008 · 2022-02-07T18:41:05Z

I've added some new e2e tests for both the Confirm and Cancel buttons on both large and small viewports.

The tests work well for me locally, apart from a couple of oddities I'll comment on inline

chad1008 · 2022-02-07T18:43:37Z

packages/e2e-tests/specs/editor/various/post-editor-template-mode.test.js

+		await trashAllPosts( 'wp_template' );
+		await trashAllPosts( 'wp_template_part' );


I've included these calls because I see they're common to a number of other template-related tests, but when the actual test runs they don't actually trash anything. The URLs this utility points to for templates and template-parts just load a WP permissions error.

I'm not sure yet if these are in place for a planned future UI or if they're artifacts of something that was removed/never implemented.

My gut feeling is that these functions may be useful as a "reset" in case multiple e2e tests were to run on the same instance (although I'm not sure about it)

Yeah, that's how it seems to me as well, it was just odd that they point to wp-admin pages that don't seem to exist/can't be accessed by an Admin. Hopefully the fix @talldan mentions below will make them operational, as cleanup of existing templates would be awesome!

chad1008 · 2022-02-07T18:47:02Z

packages/e2e-tests/specs/editor/various/post-editor-template-mode.test.js

+				await closeDocumentSettingsButton.click();
+			}
+			await insertBlock( 'Paragraph' );
+			await page.keyboard.type(


The previous test in the suite leaves the site on Twenty Twenty-one, which works well for the tests I've added. I did notice, however that switching to another theme (tested Twenty Twenty-two and Emptytheme) causes the new tests to fail (see line 206 for the e2e-util that handles the theme switch)

On either of those two themes, and potentially others, this call to page.keyboard.type isn't able to complete. It stops midway through the string for reasons I can't see and the test ends up failing.

talldan · 2022-02-08T04:16:05Z

The new tests were flagged as flaky:

[Flaky Test] should retain template if deletion is canceled when the viewport is large #38595
[Flaky Test] should delete template if deletion is confirmed when the viewport is large #38594

I'm not sure if that's related to the permissions error you noticed. There's an incoming fix for that permissions error in #38524.

ciampo · 2022-02-08T07:53:06Z

Hey @chad1008 , unfortunately I'm not familiar with the issues that you're facing in the e2e tests — @talldan , do you know who could we ask for help on this one?

I'm not sure if that's related to the permissions error you noticed. There's an incoming fix for that permissions error in #38524.

Thank you for letting us know, hopefully merging #38524 will help with the test flakiness.

ciampo · 2022-02-11T10:42:48Z

#38524 got merged! Let's rebase on top of trunk, solve conflicts and see if the tests flakiness goes away! (cc @Mamaduka — both this PR and #37491 include ConfirmDialog e2e tests 😄 )

chad1008 · 2022-02-16T14:44:37Z

Rebasing this branch to incorporate the template deletion updates did eliminate those permissions errors, but it didn't do much that I could tell for the flakiness. I have since dug in a bit more and made some improvements.

All tests in this suite are now passing consistently for me now, but happy to revisit if more flakiness surfaces!

@talldan is there a method I can use to retrigger the checks that originally flagged these tests as flaky?

ciampo · 2022-02-22T08:00:41Z

Hey @chad1008, can we consider the flakiness of the tests fixed? Asking just because I lost track with all the parallel e2e work that happened in the last week, and wasn't sure if any of that contributed to fix this PR's e2e flakiness 😅

chad1008 · 2022-02-22T10:54:17Z

@ciampo I hope so! Between the improvements I made last week and possibly coinciding changes elsewhere that may have helped, these tests pass consistently for me locally (I can run them at least 25 times in a row with no issues).

I'm no sure if we can rerun the checks that @talldan mentioned previously, or if they automatically rerun when we add new commits, but as long as there are no other indicators like those that these tests are still a problem, I think the flakiness has been resolved 🤞

Mamaduka · 2022-02-22T11:04:43Z

@chad1008, let's rebased and confirm all checks are green on Github + check for new logs in the issue of the flaky tests.

If we're clean on those fronts, I think flaky tests were resolved for this PR.

…ialog`

…it mode

ciampo · 2022-02-22T13:30:21Z

let's rebased and confirm all checks are green on Github + check for new logs in the issue of the flaky tests.

All CI checks passed successfully, and I didn't spot any new "Flaky Test" issue regarding this PR. @Mamaduka , do you know if we should wait a bit longer to see if any tests are going to be flagged as flaky (or if I didn't check in the right place 😅 )?

Otherwise I assume we're good to go!

Mamaduka · 2022-02-22T13:42:20Z

I think we're good to merge 👍 The only issue that got updated after rebase is #35502, and this test is known to be flaky.

chad1008 · 2022-02-22T13:49:30Z

Thank you both!

ciampo self-requested a review January 3, 2022 16:19

ciampo assigned chad1008 Jan 3, 2022

ciampo added [Feature] Component System WordPress component system [Package] Components /packages/components labels Jan 3, 2022

ciampo requested a review from mirka January 3, 2022 16:20

chad1008 force-pushed the migrate/post-template-delete-confirm-dialog branch from faf8885 to cf8b058 Compare January 7, 2022 21:27

ciampo reviewed Jan 9, 2022

View reviewed changes

ciampo requested a review from fullofcaffeine January 9, 2022 17:44

chad1008 mentioned this pull request Jan 13, 2022

Migrate post privacy confirmation from confirm() to ConfirmDialog #37602

Merged

7 tasks

ciampo mentioned this pull request Jan 21, 2022

Migrate the Post 'Switch to draft' button to ConfirmDialog component #37491

Merged

7 tasks

chad1008 force-pushed the migrate/post-template-delete-confirm-dialog branch from 4dcd72d to 6b40d98 Compare February 7, 2022 17:26

chad1008 requested review from ajitbohra, nerrad and ntwb as code owners February 7, 2022 18:25

chad1008 commented Feb 7, 2022

View reviewed changes

ciampo mentioned this pull request Feb 10, 2022

Tests: Use REST API to delete templates and template parts #38524

Merged

8 tasks

chad1008 force-pushed the migrate/post-template-delete-confirm-dialog branch from 2eca5e6 to 4f74617 Compare February 16, 2022 14:41

chad1008 added 11 commits February 22, 2022 07:15

migrate post template menu item from confirm() to ConfirmDialog

9e20b87

migrate the Delete template menu item from confirm() to `ConfirmD…

4a4c836

…ialog`

simplify ConfirmDialog state using the built in isOpen prop

e0eb256

explicitly close ConfirmDialog after confirmation

edc02bd

add e2e tests for Delete Template ConfirmDialog

a328a83

replace editor animation that was removed for local testing

17a2894

implement new deleteAllTemplates() util

ec2d19b

simplify xpath for increased resiliancy when switching to template ed…

34d2c45

…it mode

save and reload post to ensure template dropdown is properly updated

bf59df0

replace an unneeded reload with a more user-relatable button click

c79ff28

ensure small viewports are able to open/close settings panel correctly

415ef76

chad1008 force-pushed the migrate/post-template-delete-confirm-dialog branch from 4f74617 to 415ef76 Compare February 22, 2022 12:16

ciampo approved these changes Feb 22, 2022

View reviewed changes

ciampo merged commit 5c980de into WordPress:trunk Feb 22, 2022

github-actions bot added this to the Gutenberg 12.7 milestone Feb 22, 2022

chad1008 deleted the migrate/post-template-delete-confirm-dialog branch February 22, 2022 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Post Template Delete button from `confirm()` to `ConfirmDialog` #37535

Migrate Post Template Delete button from `confirm()` to `ConfirmDialog` #37535

chad1008 commented Dec 20, 2021 •

edited

Loading

chad1008 commented Jan 7, 2022

ciampo left a comment

chad1008 commented Jan 13, 2022

ciampo commented Jan 21, 2022 •

edited

Loading

ciampo commented Feb 4, 2022 •

edited

Loading

chad1008 commented Feb 7, 2022

chad1008 Feb 7, 2022

ciampo Feb 8, 2022 •

edited

Loading

chad1008 Feb 9, 2022

chad1008 Feb 7, 2022

talldan commented Feb 8, 2022

ciampo commented Feb 8, 2022

ciampo commented Feb 11, 2022

chad1008 commented Feb 16, 2022

ciampo commented Feb 22, 2022

chad1008 commented Feb 22, 2022

Mamaduka commented Feb 22, 2022

ciampo commented Feb 22, 2022

Mamaduka commented Feb 22, 2022

chad1008 commented Feb 22, 2022

		await trashAllPosts( 'wp_template' );
		await trashAllPosts( 'wp_template_part' );

Migrate Post Template Delete button from confirm() to ConfirmDialog #37535

Migrate Post Template Delete button from confirm() to ConfirmDialog #37535

Conversation

chad1008 commented Dec 20, 2021 • edited Loading

Description

How has this been tested?

Checklist:

chad1008 commented Jan 7, 2022

ciampo left a comment

Choose a reason for hiding this comment

chad1008 commented Jan 13, 2022

ciampo commented Jan 21, 2022 • edited Loading

ciampo commented Feb 4, 2022 • edited Loading

chad1008 commented Feb 7, 2022

chad1008 Feb 7, 2022

Choose a reason for hiding this comment

ciampo Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

chad1008 Feb 9, 2022

Choose a reason for hiding this comment

chad1008 Feb 7, 2022

Choose a reason for hiding this comment

talldan commented Feb 8, 2022

ciampo commented Feb 8, 2022

ciampo commented Feb 11, 2022

chad1008 commented Feb 16, 2022

ciampo commented Feb 22, 2022

chad1008 commented Feb 22, 2022

Mamaduka commented Feb 22, 2022

ciampo commented Feb 22, 2022

Mamaduka commented Feb 22, 2022

chad1008 commented Feb 22, 2022

Migrate Post Template Delete button from `confirm()` to `ConfirmDialog` #37535

Migrate Post Template Delete button from `confirm()` to `ConfirmDialog` #37535

chad1008 commented Dec 20, 2021 •

edited

Loading

ciampo commented Jan 21, 2022 •

edited

Loading

ciampo commented Feb 4, 2022 •

edited

Loading

ciampo Feb 8, 2022 •

edited

Loading